New windows impl#121
Conversation
9aecbb4 to
a6e43dd
Compare
a6e43dd to
e12e7b6
Compare
f16f0c4 to
54730c6
Compare
|
@madsmtm Hey do you mind giving me a quick code review here? The |
Hi @Xaeroxe, |
madsmtm
left a comment
There was a problem hiding this comment.
I think the unsafe part looks fine.
Whether which should use winsafe or not is a complex discussion. I tend to lean towards "no", for a few reasons:
- The
whichcrate is quite widely used, so any dependencies it has is dependencies that users are gonna have. - The
winsafecrate isn't that widely used, so the dependency isn't going to be shared with other dependencies, and there's security implications with the increased supply chain. - The
windows-rscrates (windows-linkin this case) are widely used, and directly from Microsoft, so the above issues aren't really a problem there.
There are benefits to winsafe, especially if which was using a bunch of Windows functions, but it isn't, it's using two simple functions, hence why I think it makes sense to not depend on it.
The same arguments apply for the rustix crate, it's used in one place so using libc directly might be the better option, though less so because it's also more popular.
(@rodrigocfd I hope you understand that I mean no slight against you or your crate, I'm only commenting on factors largely outside your control. I also get that it's a chicken and egg problem, from winsafe's perspective, you really want which as a depender, since it'll grow your download count and probably userbase, which should make the "winsafe isn't widely used" be less of a problem for others).
|
@madsmtm Yes, I would love to have Anyway, since all you guys need is 2 rather simple calls, it seems that you're still adding unnecessary complexity with You can reference the 2 functions directly: #[link(name = "kernel32")]
unsafe extern "system" {
pub fn GetBinaryTypeW(app_name: *const u16, bin_type: *mut u32) -> i32;
pub fn GetLastError() -> u32;
}That means zero dependencies for Windows. Let me know if you need any help with this. |
|
There's also another perspective to consider which is probably only apparent to me as a long term maintainer.
From a feature standpoint, Thank you for being understanding of this @rodrigocfd and I support your mission to make the usage of Windows APIs safer for us all! |
05b42a2 to
78540b6
Compare
78540b6 to
61c1656
Compare
Moves the crate into a virtual workspace, then adds a small code generation binary for generating Windows FFI bindings. These bindings are then committed to the repo and used in the which crate. The code generation binary is not intended for distribution.
This is considered preferable because
winsafe's current version is using an MSRV of 1.87, which is faster than I want to movewhich's MSRV at this time. Older versions ofwinsafeare suffering from rodrigocfd/winsafe#185Additionally we only need two functions from Windows, so bringing in most of the API bindings feels excessive.